Minor bug fixes and docs improvements #873
Conversation
I wrote the |
|
Everything else, I totally agree |
| if (html) { | ||
| // html will take precedence | ||
| setIsHtmlContent(true) | ||
| setTooltipContent(html) |
There was a problem hiding this comment.
I have the feeling that we will spend resources (a bit) doing the:
setIsHtmlContent(false)
and after that:
setIsHtmlContent(true)
In this case, I prefer not to do the setIsHtmlContent(false) if we will do setIsHtmlContent(true) two lines below.
Is this necessary in this order? or can we add a validation to do one or other?
There was a problem hiding this comment.
The validation would only serve to make the code clearer (which I think is unnecessary, since it's simple enough).
React 18 batches useState() updates, so the state is effectively only updated once. This source is a bit outdated, but I'm pretty sure that still holds true.
We can come back to this in the future if we ever decide to improve the content/html props (maybe there shouldn't be a single state variable - tooltipContent - that holds the value for the one being actually used).
Closes #872
Closes #875
After agreeing on these the PR should be ready to review.
State/Uncontrolled tooltip state section
Should we remove this section? It just shows a limitation of the component (which could be fixed, but why?), and the use case in the example can be much more conveniently achieved by using the recently added provider.
Click event
Since the recent design change (when clicking an open tooltip with
clickevent enabled, it no longer closes the tooltip), does it even make sense to use bothhoverandclickevent simultaneously? Maybe we should remove the last section.Further considerations (slightly related to PR)
I propose a breaking API change (maybe initially just deprecate
eventsinstead of fully removing it) similar to how V4seffect: 'solid' | 'float'prop got replaced by just thefloat: booleanprop.A
click: booleanprop would replace theeventsprop to activate the "on click" mode.